Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pydantic ColocationSetup #1120

Merged
merged 49 commits into from
Jun 1, 2024
Merged

Pydantic ColocationSetup #1120

merged 49 commits into from
Jun 1, 2024

Conversation

lewisblake
Copy link
Member

@lewisblake lewisblake commented Apr 14, 2024

Change Summary

This PR does quite a bit. Changes include:

  • Implement ColocationSetup in terms of Pydantic. Move this class into it's own module: colocation_setup.py
  • Colocator is no longer a child class of ColocationSetup. To get an instance of this class you must provide and instance of ColocationSetup or a dictionary. The option to pass a dictionary comes with a deprecation warning that it will be removed in future versions. Many tests have been changed to reflect this preferred method of instantiating a Colocator, where first one declares an instance of ColocationSetup and then passes this to Colocator. Dependency injection was preferred in this case to class inheritance in favor or simplifying the code between classes which contain data and classes which manipulate data.
  • Many data types in ColocationSetup have been defined more rigorously based on the expected behavior in the code. For example, obs_vars is type-hinted to be a tuple (of strings) to indicate to programmers that it is immutable. start and stop if provided as strings will be coerced into pandas Timestamps. Many of these changes have been changed in config files.
  • EvalSetup now has it's colocation_opts created using same method that other pydantic-based attributes ge filled. This is an area for future improvement once the config file structure is decided.
  • get_colocator has been hacked in a way to get this work, and probably could be made better. That said. this function goes against how I think a simpler version of pyaeroval would work. So some design should take place so that it isn't needed.
  • Tests which tested that the old style of using the Colocator and ColocationSetup have been removed, because this behavior is no longer supported nor encouraged.

It is worth noting that ColocationSetup ideally would be immutable. I have left comments in the Pydantic ConfigDict of this class to keep it in mind that this should be a future goal. Once you define a collocation setup at the beginning of your experiment, that should be it. And additional data that get discovered along the way should be placed somewhere else. Also I would like validate_assignment=True to be enabled, but it is currently commented out as it breaks the CAMS2_83 CLI, and so more work would be needed to make this change and probably some changes on the website.

Related issue number

Part of #1085. Works towards defining the interface for a ColocatedData object which we will need data validation on (#857).

Checklist

  • Start with a draft-PR
  • The PR title is a good summary of the changes
  • PR is set to AeroTools and a tentative milestone
  • Documentation reflects the changes where applicable
  • Tests for the changes exist where applicable
  • Tests pass locally
  • Tests pass on CI
  • At least 1 reviewer is selected
  • Make PR ready to review

@lewisblake lewisblake changed the title Pydantic DolocationSetup Pydantic ColocationSetup Apr 14, 2024
Copy link

codecov bot commented Apr 14, 2024

Codecov Report

Attention: Patch coverage is 81.45695% with 56 lines in your changes missing coverage. Please review.

Project coverage is 79.24%. Comparing base (6189c5b) to head (a88fe87).
Report is 584 commits behind head on main-dev.

Files with missing lines Patch % Lines
pyaerocom/colocation_setup.py 78.67% 29 Missing ⚠️
pyaerocom/colocation_auto.py 80.45% 26 Missing ⚠️
pyaerocom/helpers.py 66.66% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           main-dev    #1120      +/-   ##
============================================
+ Coverage     78.82%   79.24%   +0.41%     
============================================
  Files           128      129       +1     
  Lines         20209    20213       +4     
============================================
+ Hits          15930    16018      +88     
+ Misses         4279     4195      -84     
Flag Coverage Δ
unittests 79.24% <81.45%> (+0.41%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lewisblake lewisblake added the CAMS2_82 Issues related to the CAMS2_82 contract label Apr 16, 2024
@lewisblake lewisblake mentioned this pull request Apr 22, 2024
9 tasks
@lewisblake lewisblake added this to the m2024-06 milestone May 21, 2024
@lewisblake lewisblake marked this pull request as ready for review May 31, 2024 11:12
Copy link
Member

@heikoklein heikoklein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Minimal rather internal api-changes, i.e. col.model_id -> col.colocation_setup.model_id and eval_setup.colocation_opts["start"] to eval_setup.colocation_opts.start. Please document these.

Some no longer needed comments are still in the code and should be removed.

pyaerocom/aeroval/setupclasses.py Outdated Show resolved Hide resolved
pyaerocom/aeroval/setupclasses.py Outdated Show resolved Hide resolved
tests/fixtures/mscw_ctm.py Outdated Show resolved Hide resolved
@lewisblake
Copy link
Member Author

Looks good. Minimal rather internal api-changes, i.e. col.model_id -> col.colocation_setup.model_id and eval_setup.colocation_opts["start"] to eval_setup.colocation_opts.start. Please document these.

Some no longer needed comments are still in the code and should be removed.

Thanks. I just checked and (1) still works, (2) only occurs in private methods. Should we still document this given this class is based on pydantic and pydantic doesn't allow this functionality?

@heikoklein
Copy link
Member

If (2) only occurs in private methods, we don't need to document or implement the no-longer existing dict-like behaviour.

@heikoklein heikoklein self-requested a review May 31, 2024 15:33
@lewisblake lewisblake merged commit 5a686b0 into main-dev Jun 1, 2024
8 checks passed
@lewisblake lewisblake deleted the pydantic-colocationsetup branch July 11, 2024 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CAMS2_82 Issues related to the CAMS2_82 contract
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants